ZA | 25-SDC-July | Luke Manyamazi | Sprint 4 | Python Implement Shell Tools Exercises by Luke-Manyamazi · Pull Request #148 · CodeYourFuture/Module-Tools

Luke-Manyamazi · 2025-08-05T14:03:46Z

Learners, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist

This PR implements basic versions of the following Unix commands in Python:

cat: Concatenates and displays file contents, with support for -n (number all lines) and -b (number non-empty lines).
ls: Lists directory contents, supporting options for showing hidden files and listing in one column.
wc: Counts lines, words, and bytes in a file, with flags to show specific counts.

These scripts were designed to mimic standard command-line tools and handle typical edge cases like missing files and glob patterns.

Questions

Are there any improvements you would suggest for handling edge cases (e.g., invalid file paths, empty files)?
Is the use of argparse and glob in each script appropriate and idiomatic?
Would you recommend any refactoring for readability or modularity?
Are there any Python best practices I may have missed?

illicitonion

This is generally looking good, but I left a few things to think about :)

illicitonion · 2025-08-06T17:34:19Z

implement-shell-tools/cat/cat.py

+import glob
+import argparse
+
+def cat(filepath, n=False, b=False, line_counter=None):


n and b are both:

Not very expressive names

Seem mutually exclusive - it doesn't make sense for someone to pass both.

Can you think of a way of passing this information into the function which is easier to read, and makes more clear that only one of these should be set?

illicitonion · 2025-08-06T17:36:11Z

implement-shell-tools/cat/cat.py

+                    else:
+                        print(line, end='')
+                elif n:
+                    print(f"{line_counter[0]:6}\t{line}", end='')


There's some repetition here between the b case and n case - imagine we wanted to start padding the line number by 8 instead of 6 characters - we'd need to change that in two places. Can you think how to avoid that?

We can create a single helper function for printing a numbered line. This avoids duplicating the f"{line_counter[0]:6}" formatting. Then in cat(), both the “all lines” and “non-empty lines” cases can just call this helper.

illicitonion · 2025-08-06T17:36:56Z

implement-shell-tools/cat/cat.py

+
+    files = []
+    for pattern in args.files:
+        files.extend(glob.glob(pattern) or [pattern])


It's interesting you're globbing here - I would expect a shell to handle this. What would break if you removed the glob expansion yourself?

illicitonion · 2025-08-06T17:53:00Z

implement-shell-tools/cat/cat.py

+                if b:
+                    if line.strip():
+                        print(f"{line_counter[0]:6}\t{line}", end='')
+                        line_counter[0] += 1


Why is this value in an array? You always seem to look at exactly one value in the array?

illicitonion · 2025-08-06T17:54:37Z

implement-shell-tools/cat/cat.py

+import glob
+import argparse
+
+def cat(filepath, n=False, b=False, line_counter=None):


It looks like you always pass values for these arguments - why are you supplying default values for them? Particularly for line_counter where if None is used your code will actually break.

illicitonion · 2025-08-06T17:55:30Z

implement-shell-tools/cat/cat.py

+    except FileNotFoundError:
+        print(f"cat: {filepath}: No such file or directory")
+
+def main():


How did you test this implementation?

I created two files and tried using cat -n /file/1 /file/2 and cat -b /file/1 /file/2 and compared the output with using your script, and didn't always get the same results

This comment still stands - I get different results between your program and the builtin cat.

illicitonion · 2025-08-06T17:55:51Z

implement-shell-tools/ls/ls.py

+import os
+import argparse
+
+def ls(path='.', one_column=False, show_hidden=False):


Same question as above about default args

illicitonion · 2025-08-06T17:58:25Z

implement-shell-tools/wc/wc.py

+        if count_words: parts.append(str(len(words)))
+        if count_bytes: parts.append(str(bytes_))
+
+        if not parts:


This works, but it feels like it's testing for a side-effect ("We didn't add any paths") rather than the cause ("you didn't ask for any of lines, words, or bytes") - I'd maybe rewrite this if to be framed in what the user requested.

(But the code works fine as it is, too)

illicitonion · 2025-08-06T17:59:18Z

implement-shell-tools/wc/wc.py

+    parser.add_argument('paths', nargs='+', help='Files to count')
+    args = parser.parse_args()
+
+    for path in args.paths:


If I pass multiple files, the real wc outlines a line that you don't - can you add that too?

…for all the files

Luke-Manyamazi · 2025-10-13T19:17:53Z

cat.py

Removed the mutable list [1] hack for line numbering; now cat() returns the updated line number.
Introduced a Numbering enum (NONE, ALL, NONEMPTY) to make it explicit which numbering mode is active.
Added a helper function print_numbered_line() to avoid repeating the line number formatting logic.
Enforced mutual exclusivity of -n and -b in the argument parser.
Keeps numbering consistent across multiple files.

ls.py
Removed default values for one_column and show_hidden in the function signature; main() now passes them explicitly.
Added handling for NotADirectoryError to mimic real ls behavior.
Retained sorting and optional hidden file filtering.

wc.py
Replaced the “side-effect” check (if not parts) with an explicit check of what the user requested (-l, -w, -c).
Returns counts from wc() to allow aggregation for multiple files.
Prints totals if multiple files are provided, mimicking standard wc output.
Handles file-not-found and directory errors gracefully.

Luke-Manyamazi · 2025-10-15T14:01:38Z

Hi @illicitonion, please kindly review and see if all the requirements were met. If so please mark the PR as complete. Thank you.

illicitonion

This is generally looking good, but there are a bunch of small details and consistencies to look at :)

illicitonion · 2025-10-15T14:26:44Z

implement-shell-tools/cat/cat.py

+                else:
+                    print(line, end='')
+    except FileNotFoundError:
+        print(f"cat: {filepath}: No such file or directory")


Does this print to stdout or stderr? Which should it print to? Why?

illicitonion · 2025-10-15T14:27:03Z

implement-shell-tools/cat/cat.py

+    except FileNotFoundError:
+        print(f"cat: {filepath}: No such file or directory")
+
+def main():


This comment still stands - I get different results between your program and the builtin cat.

illicitonion · 2025-10-15T14:28:43Z

implement-shell-tools/ls/ls.py

+    except NotADirectoryError:
+        print(f"ls: cannot access '{path}': Not a directory")
+
+def main():


Same question about testing here - If I run ls -a I get different files listed than if I run python3 ls.py -a.

illicitonion · 2025-10-15T14:28:57Z

implement-shell-tools/ls/ls.py

+        print(f"ls: cannot access '{path}': No such file or directory")
+    except NotADirectoryError:
+        print(f"ls: cannot access '{path}': Not a directory")


Same question about stdout vs stderr

illicitonion · 2025-10-15T14:30:19Z

implement-shell-tools/ls/ls.py

+        print(f"ls: cannot access '{path}': No such file or directory")
+    except NotADirectoryError:
+        print(f"ls: cannot access '{path}': Not a directory")


Stretch goal: What happens if you run ls /some/file for a file not a directory? What does your program do?

illicitonion · 2025-10-15T14:51:51Z

implement-shell-tools/wc/wc.py

+    parser.add_argument('paths', nargs='+', help='Files to count')
+    args = parser.parse_args()
+
+    total_lines = total_words = total_bytes = 0


It's generally pretty unusual to initialise multiple variables like this in one go, because it makes it harder to skim the code to see where a particular variable was set/initialised than initialising each on its own line.

illicitonion · 2025-10-15T14:52:35Z

implement-shell-tools/wc/wc.py

+
+    if multiple_files:
+        parts = []
+        if args.l or not any([args.l, args.w, args.c]): parts.append(str(total_lines))


You're repeating this not any([args.l, args.w, args.c]) three times - can you think of a way to avoid that duplication?

illicitonion · 2025-10-15T14:53:32Z

implement-shell-tools/wc/wc.py

+
+        # Determine what to show
+        if not any([count_lines, count_words, count_bytes]):
+            count_lines = count_words = count_bytes = True


Again, I'd generally avoid putting this all on one line

illicitonion · 2025-10-15T15:00:03Z

implement-shell-tools/wc/wc.py

+
+        print(' '.join(parts), path)
+
+        return (len(lines) if count_lines else 0,


It's interesting that you're doing this filtering twice - you're checking whether we should output lines both inside this function and when printing the totals.

Personally, I would probably just always return the values here, and let the caller decide whether to show them.

illicitonion · 2025-10-15T15:07:26Z

implement-shell-tools/wc/wc.py

+        if count_lines: parts.append(str(len(lines)))
+        if count_words: parts.append(str(len(words)))
+        if count_bytes: parts.append(str(bytes_))


It's a little unusual that two of these have len calls and the last doesn't - not a big deal, but a bit oddly inconsistent - I would maybe make these consistent.

Luke-Manyamazi added 4 commits August 1, 2025 05:15

added python the code for the cat command

3a31fbf

added python code for the ls command

d443669

added a destination to handel the -1 as a string

1900a28

added python code for the wc exercise

ff86f10

Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 5, 2025

illicitonion added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 6, 2025

illicitonion reviewed Aug 6, 2025

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 6, 2025

Luke-Manyamazi linked an issue Aug 7, 2025 that may be closed by this pull request

01 Implement shell tools (cat, ls, wc) in Python Luke-Manyamazi/My-Coursework-Planner#177

Open

Luke-Manyamazi force-pushed the python-shell-tools-exercises branch from 35a4349 to ff86f10 Compare August 14, 2025 11:09

replace -n/-b flags with Numbering Enum

fcac82c

Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2025

refactored and corrected code to meet and answer the asked questions …

5476f45

…for all the files

illicitonion reviewed Oct 15, 2025

View reviewed changes

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 2025


		print(' '.join(parts), path)

		return (len(lines) if count_lines else 0,

Uh oh!

Conversation

Luke-Manyamazi commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luke-Manyamazi commented Oct 13, 2025

Uh oh!

Luke-Manyamazi commented Oct 15, 2025

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Luke-Manyamazi commented Aug 5, 2025 •

edited

Loading